Skip to content

BUG: Fix cloning of objects that require constructor arguments - #4262

Merged
stefan6419846 merged 4 commits into
py-pdf:mainfrom
Ak4shh:fix/clone-annotation-dictionaries
Nov 3, 2026
Merged

BUG: Fix cloning of objects that require constructor arguments#4262
stefan6419846 merged 4 commits into
py-pdf:mainfrom
Ak4shh:fix/clone-annotation-dictionaries

Conversation

@Ak4shh

@Ak4shh Ak4shh commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #3467.

The bug

PageObject.merge_page() clones the annotations of the page passed to it.
DictionaryObject.clone() built the clone target with self.__class__(),
which is impossible for every annotation class, because their constructors
require arguments:

TypeError: Polygon.__init__() missing 1 required positional argument: 'vertices'

The same self.__class__() call was in DictionaryObject.replicate(), so this
affected any clone/replicate of an annotation dictionary, not just
merge_page.

The fix

DictionaryObject._new_for_clone() creates the empty target. When the class
cannot be instantiated without arguments, it instantiates it without calling
__init__; the clone is populated from the source object immediately
afterwards, so the constructor arguments are not needed. Annotation
constructors only store their arguments in the dictionary anyway
(self["/Vertices"] = ...), which _clone then copies from the source.

The check is signature-based (and cached per class) rather than
try/except TypeError, because Link() raises ValueError when neither
url nor target_page_index is given.

Classes that can be built without arguments (e.g. PageObject, which sets
pdf in __init__) keep taking the normal construction path.

Tests

  • tests/test_page.py::test_merge_page_with_annotations_of_merged_page — the
    issue's reproducer: merge a page that carries a Polygon annotation, then
    assert the annotation survives a write/re-read round trip.
  • tests/test_annotations.py::test_clone_and_replicate_annotation —
    parametrized over all ten annotation classes (Ellipse, FreeText,
    Highlight, Line, Link, Polygon, PolyLine, Popup, Rectangle,
    Text), asserting that clone() and replicate() both return the same
    class with the same keys and that the copy does not share entries with the
    source.

Both tests fail on main and pass with this change:

# without the fix
TypeError: Ellipse.__init__() missing 1 required positional argument: 'rect'
TypeError: FreeText.__init__() missing 2 required keyword-only arguments: ...
TypeError: Highlight.__init__() missing 2 required keyword-only arguments: ...
TypeError: Line.__init__() missing 3 required positional arguments: ...

Full suite

python -m pytest tests -q --basetemp=... on Windows / Python 3.13:
31 failed, 2158 passed, 34 skipped, 95 errors — identical failures and errors
to the unmodified tree (all pre-existing and environmental: empty
sample-files/ submodule, missing fontTools, pytest-cache/tmp_path
permission issues). passed rises by exactly the 11 new test cases.

Scope

Three files: pypdf/generic/_data_structures.py, tests/test_annotations.py,
tests/test_page.py.

@Ak4shh
Ak4shh force-pushed the fix/clone-annotation-dictionaries branch from c0712a9 to ac5cef6 Compare October 27, 2026 01:15
@codecov

codecov Bot commented Oct 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.81%. Comparing base (affcf3a) to head (bc3d470).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4262      +/-   ##
==========================================
+ Coverage   98.79%   98.81%   +0.01%     
==========================================
  Files          59       59              
  Lines       12120    12130      +10     
  Branches     2308     2309       +1     
==========================================
+ Hits        11974    11986      +12     
  Misses         70       70              
+ Partials       76       74       -2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

stefan6419846
stefan6419846 requested changes Oct 28, 2026
View reviewed changes
Comment thread pypdf/generic/_data_structures.py Resolved
Comment thread tests/test_annotations.py Resolved
Comment thread tests/test_page.py Resolved
@Ak4shh
Ak4shh force-pushed the fix/clone-annotation-dictionaries branch from ac5cef6 to 9bc7dd5 Compare October 29, 2026 01:37
@Ak4shh

Ak4shh commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Removed the try/except around inspect.signature(). A Python-level init never triggers it, so it was dead code.
The annotation test now compares .items(), so values are checked as well as keys.
The test docstrings now describe only the behavior under test, not the old behavior.

@Ak4shh

Ak4shh commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed all three points, and also fixed cloning of linked /Next//Prev//N//V objects (with a test). Could you take another look?

@MartinThoma MartinThoma mentioned this pull request Nov 2, 2026
Merged
MartinThoma added a commit that referenced this pull request Nov 2, 2026
@MartinThoma @claude
TST: Cover content stream renaming, scaling and merging of PageObject
5dd769b 
Add tests for PageObject branches without coverage so far:

* _content_stream_rename() with an inline image: renaming a color space
  resource called /G renames "/G cs", but keeps /CS /G in the inline
  image, where it means DeviceGray,
* scale() of a page with a /VP viewport, given as array and as
  dictionary,
* extract_text() passing text to visitor_text that is not terminated by
  an ET operator.

Two strict xfail tests document bugs in merge_page():

* #4335: merging into a page whose /Annots is null silently drops the
  annotations of the merged page,
* #3467: merging a page whose annotations were added with
  PdfWriter.add_annotation() raises TypeError: clone() creates the copy
  with self.__class__(), but Highlight, FreeText, ... have required
  keyword-only arguments. The open PRs #3785 and #4262 fix it.

Where the expected behavior comes from ISO 32000-2:2020, the docstring
names the section.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stefan6419846
stefan6419846 requested changes Nov 2, 2026
View reviewed changes

@stefan6419846 stefan6419846 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the merge conflicts.

MartinThoma added a commit that referenced this pull request Nov 2, 2026
@MartinThoma @claude
TST: Cover content stream renaming, scaling and merging of PageObject
b4b6b6d 
Add tests for PageObject branches without coverage so far:

* _content_stream_rename() with an inline image: renaming a color space
  resource called /G renames "/G cs", but keeps /CS /G in the inline
  image, where it means DeviceGray,
* scale() of a page with a /VP viewport, given as array and as
  dictionary,
* extract_text() passing text to visitor_text that is not terminated by
  an ET operator.

Two strict xfail tests document bugs in merge_page():

* #4335: merging into a page whose /Annots is null silently drops the
  annotations of the merged page,
* #3467: merging a page whose annotations were added with
  PdfWriter.add_annotation() raises TypeError: clone() creates the copy
  with self.__class__(), but Highlight, FreeText, ... have required
  keyword-only arguments. The open PRs #3785 and #4262 fix it.

Where the expected behavior comes from ISO 32000-2:2020, the docstring
names the section.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stefan6419846 pushed a commit that referenced this pull request Nov 2, 2026
@MartinThoma @claude
TST: Cover content stream renaming, scaling and merging of PageObject (…
dbe8728 
…#4338)

Add tests for PageObject branches without coverage so far:

* _content_stream_rename() with an inline image: renaming a color space
  resource called /G renames "/G cs", but keeps /CS /G in the inline
  image, where it means DeviceGray,
* scale() of a page with a /VP viewport, given as array and as
  dictionary,
* extract_text() passing text to visitor_text that is not terminated by
  an ET operator.

Two strict xfail tests document bugs in merge_page():

* #4335: merging into a page whose /Annots is null silently drops the
  annotations of the merged page,
* #3467: merging a page whose annotations were added with
  PdfWriter.add_annotation() raises TypeError: clone() creates the copy
  with self.__class__(), but Highlight, FreeText, ... have required
  keyword-only arguments. The open PRs #3785 and #4262 fix it.

Where the expected behavior comes from ISO 32000-2:2020, the docstring
names the section.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Ak4shh and others added 3 commits November 2, 2026 19:02
@Ak4shh
Fix cloning of objects that require constructor arguments
0a52ef2 
DictionaryObject.clone() and DictionaryObject.replicate() created the target
object via self.__class__(). That fails for every annotation class (Text,
FreeText, Link, Polygon, ...), because their constructors require arguments,
so PageObject.merge_page() raised TypeError whenever the merged page carried a
markup annotation.

Create the empty target through a helper that skips __init__ when the class
cannot be instantiated without arguments. The clone is populated from the
source object right afterwards, so the constructor arguments are not needed;
the annotation constructors only store their arguments in the dictionary
anyway.

Fixes py-pdf#3467.

Signed-off-by: Ak4shh <akashposa40@gmail.com>
@Ak4shh @claude
Clone linked objects requiring constructor arguments
c54b421 
Linked entries such as /Next, /Prev, /N and /V were still instantiated
via `__class__()`, failing for classes like OutlineItem.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ak4shh <akashposa40@gmail.com>
@Ak4shh @claude
Narrow optional indirect references in clone test for mypy
18da6d4 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ak4shh <akashposa40@gmail.com>
@Ak4shh
Ak4shh force-pushed the fix/clone-annotation-dictionaries branch from 7d3deae to 18da6d4 Compare November 3, 2026 00:03
@Ak4shh
TST: Remove obsolete annotation cloning xfail marker
bc3d470 
Signed-off-by: Ak4shh <akashposa40@gmail.com>
@Ak4shh
Ak4shh force-pushed the fix/clone-annotation-dictionaries branch 2 times, most recently from 65dc82f to bc3d470 Compare November 3, 2026 00:10
@stefan6419846
stefan6419846 merged commit 6a7ebac into py-pdf:main Nov 3, 2026
23 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stefan6419846 stefan6419846 stefan6419846 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

merge_page with passed page having markup annotation fails

2 participants

@Ak4shh @stefan6419846